-
-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Failing tests for IDN TLD that contains uppercase #64
Failing tests for IDN TLD that contains uppercase #64
Conversation
Alternately, we could depend on symfony/polyfill-iconv and/or symfony/polyfill-mbstring, allowing us to use the relevant iconv and/or mbstring functions directly in the code. On systems that have the extension(s) installed, these are ignored, but otherwise, we'd still be able to rely on them, without needing to write our own wrappers. |
As I see it, a strategic decision is needed here, because problems with multi-byte strings will occur over and over again in a wide variety of components. Possible options:
|
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as resolved.
This comment was marked as resolved.
@rugabarbo perhaps you assumed the polyfills that @weierophinney mentioned are Symfony components, they are not, they are absolutely framework agnostic, they are just composer packages under Symfony namespace: but if you meant really components, you mean string component? then it would require to use Symfony 5.0 which in turn requires PHP 7.2:
but this project is yet at PHP 7.1: I personally would just go using polyfills, which can be replaced with real extensions using composer "replace" key: |
1031676
to
6342fd5
Compare
I've pushed changes that demonstrate an attempt at fixing this using symfony/polyfill-mbstring. I installed that package, and then updated all string-related functions that have mbstring equivalents to the mbstring versions. When I was done, I noted the following:
Out of curiousity, I tried the same experiment using symfony/polyfill-iconv. However, in that scenario, the tests you've created continued to fail entirely. I'm not quite sure where to go with this at this point, but I've left my symfony/polfill-mbstring experiment in here for you to work with. You'll need to fetch and rebase your branch. |
…dator This patch adds symfony/polyfill-mbstring as a requirement, and updates the `Hostname` validator to use mbstring equivalents of various string operations, where they are available. Locally, one of the two new tests passes, but one original test ("UTF-8 label + UTF-8 TLD (cyrillic)") now fails — which may be a problem with my own locale. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
6342fd5
to
ebaa162
Compare
I've rebased this off current 2.14.x sources so that it can pick up our new CI. However, the tests as presented are still failing and it looks like it's likely due to the letter casing. I attempted to change the regexp to add the At this point, I need somebody with experience in i18n casing issues to assist. |
@@ -28,7 +28,8 @@ | |||
"php": "^7.3 || ~8.0.0", | |||
"container-interop/container-interop": "^1.1", | |||
"laminas/laminas-stdlib": "^3.3", | |||
"laminas/laminas-zendframework-bridge": "^1.0" | |||
"laminas/laminas-zendframework-bridge": "^1.0", | |||
"symfony/polyfill-mbstring": "^1.20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO to be declared should be ext-mbstring
, while we can decide to use symfony/polyfill-mbstring
in require-dev
These are just failing tests for top-level IDN domains that contain uppercase letters.
I tried to fix it, but found that the fix would be non-trivial. The problem is caused by using the built-in PHP functions
strtoupper()
andstrtolower()
. For example:laminas-validator/src/Hostname.php
Line 2037 in 3f5e678
These functions do not work correctly with multi-byte encodings. But for processing multi-byte encodings, the code uses wrappers in the
\Laminas\Stdlib\StringWrapper
namespace, which do not yet contain functionsstrtoupper()
andstrtolower()
.Thus, a full fix will look like this:
\Laminas\Stdlib\StringWrapper\StringWrapperInterface
methodsstrtoupper()
andstrtolower()
So this is a subject for discussion...
Maybe I misunderstood something.